Migrate log4j-slf4j2-impl to JUnit 5#3080
Conversation
Generated by gradle/develocity-actions |
ppkarwasz
left a comment
ppkarwasz
left a comment
There was a problem hiding this comment.
This looks great, thanks!
Since the log4j-slf4j-impl module is mostly identical to this one (it has a couple of tests less). Could you apply the changes to that module too?
Could you also remove the junit-vintage-engine dependency in the pom.xml to show that not tests are using JUnit 4?
| Set<LoggerContext> set = factory.getLoggerContexts(); | ||
| final LoggerContext ctx1 = set.toArray(LoggerContext.EMPTY_ARRAY)[0]; | ||
| assertTrue("LoggerContext is not enabled for shutdown", ctx1 instanceof LifeCycle); | ||
| assertTrue(ctx1 instanceof LifeCycle, "LoggerContext is not enabled for shutdown"); |
There was a problem hiding this comment.
| assertTrue(ctx1 instanceof LifeCycle, "LoggerContext is not enabled for shutdown"); | |
| assertInstanceOf(LifeCycle.class, ctx1, "LoggerContext is not enabled for shutdown"); |
Could you change the assertions on X instanceof Y to use the new assertInstanceOf assertions?
As far as I can tell <plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<executions>
<!-- Separate test execution to verify that the presence of both:
~ * `log4j-slf4j2-impl` (bridge from SLF4J to Log4j API)
~ * `log4j-to-slf4j` (bridge from Log4j API to SLF4J)
~ does not cause a stack overflow.
-->
<execution>
<id>loop-test</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<additionalClasspathDependencies>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-to-slf4j</artifactId>
<version>${project.version}</version>
</dependency>
</additionalClasspathDependencies>
<includes>
<include>**/OverflowTest.java</include>
</includes>
</configuration>
</execution>
<execution>
<id>default-test</id>
<configuration>
<includes>
<include>**/*Test.java</include>
</includes>
<excludes>
<exclude>**/OverflowTest.java</exclude>
</excludes>
</configuration>
</execution>
</executions>
</plugin>and |
AlbaHerrerias
force-pushed
the
log4j-slf4j2-impl-migrate-junit5
branch
from
c0b8144 to
4c23edd
Compare
|
Thank you for the review and assistance, I have addressed now your requested changes. Please let me know if you'd like to request more. I will make the refactor of the Also note that, with the removal of |
AlbaHerrerias
force-pushed
the
log4j-slf4j2-impl-migrate-junit5
branch
from
4c23edd to
67a954c
Compare
2 participants
Hello 👋
We are from Neighbourhoodie, the implementation partner of the STF Bug Resilience Program. This work is part of our agreed Milestone 1. Upgrade from JUnit 4 to JUnit 5. This PR migrates the tests located in
log4j-slf4j2-impl.OverflowTestis set to run specifically with Junit 4. This behaviour was added not long ago so we wanted to ask, should we leave it like this or should we refactorOverflowTestto JUnit 5?Thank you!
Checklist
2.xbranch if you are targeting Log4j 2; usemainotherwise./mvnw verifysucceeds (if it fails due to code formatting issues reported by Spotless, simply run./mvnw spotless:applyand retry)src/changelog/.2.x.xdirectory